-
Notifications
You must be signed in to change notification settings - Fork 33
MOB-11663-InApp-Calculations #949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Needs reduction in code. Lot of repition. Heavy review and testing needed. Committing as inapps are looking stable and are sticking with the layout with back to back phone rotations when inapp performance would fail usually
| private String messageId; | ||
|
|
||
| // Resize debouncing fields | ||
| private Handler resizeHandler = new Handler(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Handler.Handler
| lastOrientation = currentOrientation; | ||
|
|
||
| // Use longer delay for orientation changes to allow layout to stabilize | ||
| final Handler handler = new Handler(); |
Check notice
Code scanning / CodeQL
Deprecated method or constructor invocation Note
Handler.Handler
2cc8a75 to
8e15c0a
Compare
iterableapi/src/androidTest/java/com/iterable/iterableapi/IterableApiResponseTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial pass done
| if (loaded && webView != null) { | ||
| // Only trigger on significant orientation changes (90 degree increments) | ||
| int currentOrientation = ((orientation + 45) / 90) * 90; | ||
| if (currentOrientation != lastOrientation && lastOrientation != -1) { | ||
| lastOrientation = currentOrientation; | ||
|
|
||
| // Use longer delay for orientation changes to allow layout to stabilize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for resize script to be called only limted number of times. This method when implemented, felt it was only called when landscape to portrait rotation was made. Hence explicit conditioning is required.
iterableapi/src/main/java/com/iterable/iterableapi/IterableInAppFragmentHTMLNotification.java
Outdated
Show resolved
Hide resolved
iterableapi/src/main/java/com/iterable/iterableapi/IterableInAppFragmentHTMLNotification.java
Show resolved
Hide resolved
And removed unnecessary logging
54b0276 to
e85d78a
Compare
|
@Ayyanchira Hmm lets have a chat about these. It seems we have made too many changes related to In app screen size for this release and i want to see them all in one place somehow. Also wondering if a smaller diff will solve the problem. cc @davidtruong |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add tests around orientation calculations
| }, 1000); | ||
| if (loaded && webView != null) { | ||
| // Only trigger on significant orientation changes (90 degree increments) | ||
| int currentOrientation = ((orientation + 45) / 90) * 90; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Be careful with the unit conversions since you are using an int and not a double.
Lets pull this calculation out to its own function so we can properly put tests around it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point actually especilly /90s can give easy values around 0 in decimals. Rounding up can be quite errorsome!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets do another code review with AI and also add more tests to validate the fixes.
- View Hierarchy Mismatch During Resize
Risk Level: 🔥 CRITICAL
Issue: In resize() method line 605, you're creating RelativeLayout.LayoutParams for the WebView, but the WebView's actual parent is a RelativeLayout inside a FrameLayout. You're setting rules like CENTER_IN_PARENT, ALIGN_PARENT_TOP, but these are RelativeLayout-specific rules that apply to the RelativeLayout's coordinate system, NOT the FrameLayout's.
// Line 605 - This creates RelativeLayout paramsRelativeLayout.LayoutParams webViewParams = new RelativeLayout.LayoutParams(newWebViewWidth, newWebViewHeight);// But you're not updating the FrameLayout params at all!// The FrameLayout container was set in onCreateView with gravity, but resize() doesn't touch it
Why this breaks:
The initial layout in onCreateView() sets gravity on the FrameLayout.LayoutParams (lines 243-254)
The resize() method sets gravity via RelativeLayout rules on the WebView
These two systems could conflict or not work together as expected
Fix: Need to ensure the resize logic accounts for the FrameLayout container gravity, or you need to update BOTH the FrameLayout params AND the WebView params consistently. - Multiple applyWindowGravity() Calls May Cause Flickering
Risk Level: 🔥 HIGH
Issue: You're calling applyWindowGravity() in 3 places:
onCreateDialog() - line 174
onCreateView() - line 196
onStart() - line 130
Why this is risky:
DialogFragments have a specific lifecycle. Setting window attributes multiple times could cause the dialog to reposition or flicker
onStart() happens AFTER the dialog is shown, which means users might see the dialog jump to a different position
Android's window system may not handle redundant gravity changes gracefully across different Android versions
Likely bug: On some devices, you might see the in-app "pop" or shift position after it's already displayed.
and potentiall others.
| IterableLogger.d(TAG, "Orientation changed, triggering resize"); | ||
| runResizeScript(); | ||
| } | ||
| }, 1500); // Increased delay for better stability |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magic number
🔹 Jira Ticket(s)
✏️ Description
This PR refactors the in-app message rendering system to improve positioning stability and reliability, particularly during device rotations and dynamic content resizing. The core change migrates from a single
RelativeLayoutto aFrameLayout→RelativeLayout→WebViewhierarchy for better positioning control across TOP/CENTER/BOTTOM/FULLSCREEN layouts.Key Changes
1. Layout Hierarchy Refactor
RelativeLayoutwithsetVerticalGravity()andMATCH_PARENTsizingFrameLayout(positioning) →RelativeLayout(WebView wrapper) →WebView(content)LayoutParams.gravityWRAP_CONTENTand resizes to content heightapplyWindowGravity()method to consistently set window positioning across lifecycle stages2. Resize Debouncing & Validation
lastContentHeightto avoid unnecessary layout passes3. Improved Orientation Handling
lastOrientationtracking to prevent spurious resize triggers4. WebChromeClient Optimization
5. Test Improvements
IterableApiResponseTestby explicitly starting MockWebServerTechnical Details
Window Gravity Strategy:
onCreateDialog(),onCreateView(), andonStart()MATCH_PARENT×MATCH_PARENTto allow flexible WebView positioningResize Flow:
runResizeScript()performResizeWithValidation()checks height validity🧪 Testing Strategy
Automated Tests
getVerticalLocation()still passManual Testing Steps
Prerequisites
Test Case 1: Layout Positioning
Pass Criteria: All layouts appear in correct position without jumping or flickering
Test Case 2: Device Rotation Stability
Pass Criteria: In-app maintains correct positioning through all rotations without visual glitches
Test Case 3: Dynamic Content Resizing
Pass Criteria: Content-based resizing is smooth with minimal visual disruption
Test Case 4: Edge Cases
Pass Criteria: App remains stable under stress conditions
Test Case 5: Regression Check
Pass Criteria: No regressions in previously working functionality
🚨 Known Issues / Follow-ups
applyWindowGravity()calls may cause flickering on some devices - Consider consolidating to single call inonStart()onDestroy()only removes specific runnable - Should useremoveCallbacksAndMessages(null)for complete cleanup📸 Before/After Screenshots
🔍 Reviewer Notes
IterableInAppFragmentHTMLNotification.javaresize()method (lines 558-643) - this is where FrameLayout/RelativeLayout coordination happensrunResizeScript()andperformResizeWithValidation()should be reviewed for thread safety